Skip to content

refactor(l1): improve the way we remove ONLY the storage of an account #4850

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom
Merged
merged 18 commits into from
Oct 17, 2025
Merged

refactor(l1): improve the way we remove ONLY the storage of an account #4850

merged 18 commits into from
Oct 17, 2025

Conversation

@JereSalo
Copy link
Contributor

Motivation

  • For removing just the storage of an account we were doing something "dirty", which was inserting 2 updates for the same account. One with its destruction and another one with the changes, this cleans that up.

Description

  • Add removed_storage attribute to AccountUpdate
    • If we want to remove whole storage of an account we just change the storage root of the account to the empty root.

Closes #3321

@github-actions github-actions bot added the L1 Ethereum client label Oct 13, 2025
@JereSalo JereSalo changed the title refactor(l1): tidy the way we remove the storage of an account refactor(l1): improve the way we remove ONLY the storage of an account Oct 13, 2025
@JereSalo JereSalo moved this to In Progress in ethrex_l1 Oct 13, 2025
@github-actions
Copy link

github-actions bot commented Oct 13, 2025 •
edited
Loading

Lines of code report

Total lines added: 14
Total lines removed: 12
Total lines changed: 26

Detailed view 
+-------------------------------------------------------+-------+------+
| File                                                  | Lines | Diff |
+-------------------------------------------------------+-------+------+
| ethrex/crates/common/types/account_update.rs          | 41    | +2   |
+-------------------------------------------------------+-------+------+
| ethrex/crates/common/types/block_execution_witness.rs | 426   | +3   |
+-------------------------------------------------------+-------+------+
| ethrex/crates/common/utils.rs                         | 63    | +1   |
+-------------------------------------------------------+-------+------+
| ethrex/crates/l2/common/src/state_diff.rs             | 548   | +1   |
+-------------------------------------------------------+-------+------+
| ethrex/crates/storage/store.rs                        | 1436  | +6   |
+-------------------------------------------------------+-------+------+
| ethrex/crates/vm/levm/src/db/gen_db.rs                | 326   | -12  |
+-------------------------------------------------------+-------+------+
| ethrex/tooling/ef_tests/state/runner/revm_db.rs       | 198   | +1   |
+-------------------------------------------------------+-------+------+

@github-actions
Copy link

github-actions bot commented Oct 13, 2025 •
edited
Loading

Benchmark Results Comparison

No significant difference was registered for any benchmark run.

Detailed Results

Benchmark Results: BubbleSort

Command Mean [s] Min [s] Max [s] Relative
main_revm_BubbleSort 4.755 ± 0.011 4.746 4.783 1.02 ± 0.01
main_levm_BubbleSort 4.676 ± 0.021 4.651 4.702 1.01 ± 0.01
pr_revm_BubbleSort 4.755 ± 0.016 4.733 4.786 1.02 ± 0.01
pr_levm_BubbleSort 4.651 ± 0.023 4.612 4.681 1.00

Benchmark Results: ERC20Approval

Command Mean [s] Min [s] Max [s] Relative
main_revm_ERC20Approval 1.578 ± 0.014 1.568 1.604 1.01 ± 0.01
main_levm_ERC20Approval 1.637 ± 0.009 1.626 1.655 1.04 ± 0.01
pr_revm_ERC20Approval 1.569 ± 0.010 1.561 1.595 1.00
pr_levm_ERC20Approval 1.638 ± 0.007 1.624 1.648 1.04 ± 0.01

Benchmark Results: ERC20Mint

Command Mean [ms] Min [ms] Max [ms] Relative
main_revm_ERC20Mint 185.7 ± 0.5 184.6 186.3 1.00
main_levm_ERC20Mint 198.6 ± 2.1 196.3 202.9 1.07 ± 0.01
pr_revm_ERC20Mint 187.9 ± 0.8 186.9 189.3 1.01 ± 0.00
pr_levm_ERC20Mint 198.2 ± 2.3 196.5 204.0 1.07 ± 0.01

Benchmark Results: ERC20Transfer

Command Mean [ms] Min [ms] Max [ms] Relative
main_revm_ERC20Transfer 363.3 ± 15.3 356.6 406.6 1.01 ± 0.04
main_levm_ERC20Transfer 382.6 ± 0.9 380.6 383.7 1.06 ± 0.01
pr_revm_ERC20Transfer 360.8 ± 4.5 357.8 369.7 1.00
pr_levm_ERC20Transfer 381.9 ± 1.4 379.7 384.3 1.06 ± 0.01

Benchmark Results: Factorial

Command Mean [ms] Min [ms] Max [ms] Relative
main_revm_Factorial 242.8 ± 1.1 241.3 245.1 1.00
main_levm_Factorial 279.3 ± 2.7 276.6 285.2 1.15 ± 0.01
pr_revm_Factorial 245.1 ± 7.0 242.2 265.1 1.01 ± 0.03
pr_levm_Factorial 283.5 ± 8.9 277.5 301.4 1.17 ± 0.04

Benchmark Results: FactorialRecursive

Command Mean [s] Min [s] Max [s] Relative
main_revm_FactorialRecursive 1.711 ± 0.045 1.607 1.771 1.00
main_levm_FactorialRecursive 8.611 ± 0.688 6.679 8.998 5.03 ± 0.42
pr_revm_FactorialRecursive 1.718 ± 0.033 1.647 1.758 1.00 ± 0.03
pr_levm_FactorialRecursive 8.821 ± 0.089 8.668 8.957 5.16 ± 0.14

Benchmark Results: Fibonacci

Command Mean [ms] Min [ms] Max [ms] Relative
main_revm_Fibonacci 217.2 ± 0.4 216.8 217.9 1.00
main_levm_Fibonacci 249.2 ± 1.7 247.6 253.6 1.15 ± 0.01
pr_revm_Fibonacci 218.6 ± 6.2 213.9 235.6 1.01 ± 0.03
pr_levm_Fibonacci 249.8 ± 1.1 248.8 252.4 1.15 ± 0.01

Benchmark Results: FibonacciRecursive

Command Mean [ms] Min [ms] Max [ms] Relative
main_revm_FibonacciRecursive 908.4 ± 12.8 891.9 931.3 1.02 ± 0.02
main_levm_FibonacciRecursive 1041.5 ± 4.7 1035.2 1048.1 1.17 ± 0.01
pr_revm_FibonacciRecursive 891.4 ± 8.6 874.0 901.1 1.00
pr_levm_FibonacciRecursive 1037.1 ± 6.6 1028.4 1048.4 1.16 ± 0.01

Benchmark Results: ManyHashes

Command Mean [ms] Min [ms] Max [ms] Relative
main_revm_ManyHashes 12.6 ± 0.1 12.5 12.8 1.00
main_levm_ManyHashes 14.1 ± 0.1 14.0 14.2 1.11 ± 0.01
pr_revm_ManyHashes 12.6 ± 0.1 12.5 12.7 1.00 ± 0.01
pr_levm_ManyHashes 14.1 ± 0.2 13.9 14.5 1.12 ± 0.02

Benchmark Results: MstoreBench

Command Mean [ms] Min [ms] Max [ms] Relative
main_revm_MstoreBench 275.7 ± 2.9 271.8 279.1 1.00 ± 0.02
main_levm_MstoreBench 757.3 ± 4.0 754.0 767.9 2.75 ± 0.05
pr_revm_MstoreBench 275.5 ± 4.4 271.3 284.0 1.00
pr_levm_MstoreBench 755.3 ± 2.2 752.0 759.9 2.74 ± 0.04

Benchmark Results: Push

Command Mean [ms] Min [ms] Max [ms] Relative
main_revm_Push 303.0 ± 1.0 301.9 305.2 1.00
main_levm_Push 830.1 ± 5.0 826.2 841.7 2.74 ± 0.02
pr_revm_Push 303.9 ± 2.1 302.2 309.0 1.00 ± 0.01
pr_levm_Push 831.2 ± 1.0 829.7 832.6 2.74 ± 0.01

Benchmark Results: SstoreBench_no_opt

Command Mean [ms] Min [ms] Max [ms] Relative
main_revm_SstoreBench_no_opt 220.0 ± 0.3 219.7 220.6 2.39 ± 0.03
main_levm_SstoreBench_no_opt 93.3 ± 1.1 90.3 94.0 1.01 ± 0.02
pr_revm_SstoreBench_no_opt 219.6 ± 0.5 218.7 220.2 2.39 ± 0.03
pr_levm_SstoreBench_no_opt 92.0 ± 1.3 90.1 93.7 1.00

JereSalo
JereSalo commented Oct 14, 2025
View reviewed changes
tooling/ef_tests/state/runner/revm_db.rs
)
})
.collect(),
removed_storage: false,
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We could improve code around this so that the get state transitions of revm makes use of removed_storage but I consider it to be irrelevant, we are going to remove this code whenever we can anyway

@JereSalo JereSalo marked this pull request as ready for review October 14, 2025 19:50
@JereSalo JereSalo requested a review from a team as a code owner October 14, 2025 19:50
@ethrex-project-sync ethrex-project-sync bot moved this from In Progress to In Review in ethrex_l1 Oct 14, 2025
Base automatically changed from levm/use_account_status to main October 15, 2025 18:02
SDartayet
SDartayet approved these changes Oct 17, 2025
View reviewed changes
Copy link
Contributor

@SDartayet SDartayet left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM!

@SDartayet SDartayet added this pull request to the merge queue Oct 17, 2025
fmoletta
fmoletta reviewed Oct 17, 2025
View reviewed changes
crates/vm/levm/src/db/gen_db.rs
initial_state_account.storage.get(key).ok_or_else(|| { VMError::Internal(InternalError::Custom(format!("Failed to get old value from account's initial storage for address: {address}")))})?
} else {
// There's not an "old value" if the contract was destroyed and re-created.
&ZERO_U256
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why not U256::zero()?

Copy link
Contributor Author

@JereSalo JereSalo Oct 17, 2025 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Because it would create a reference that's dropped every time

Merged via the queue into main with commit 656d028 Oct 17, 2025
53 checks passed
@SDartayet SDartayet deleted the remove_storage_account_update branch October 17, 2025 21:39
@github-project-automation github-project-automation bot moved this from In Review to Done in ethrex_l1 Oct 17, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@mpaulucci mpaulucci mpaulucci approved these changes

@SDartayet SDartayet SDartayet approved these changes

@fmoletta fmoletta fmoletta approved these changes

Assignees

@JereSalo JereSalo

Labels

L1 Ethereum client

Projects

ethrex_l1
Status: Done

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Add remove_storage to AccountUpdate

4 participants

@JereSalo @mpaulucci @SDartayet @fmoletta